Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Discover] EuiDataGrid Implementation #67259

Merged
merged 272 commits into from
Dec 23, 2020
Merged

[Discover] EuiDataGrid Implementation #67259

merged 272 commits into from
Dec 23, 2020

Conversation

kertal
Copy link
Member

@kertal kertal commented May 25, 2020 •
edited
Loading

Summary

Continuation of #51531 🔥 integrating EuiDataGrid implementation in Discover including the saved search embeddable.

You need to enable it in Stack Monitoring / Advanced Settings by setting Use legacy table to Off

Bildschirmfoto 2020-12-07 um 17 09 09

When you navigate to Discover you should see the EuiDataGrid in Action

Bildschirmfoto 2020-12-07 um 17 14 15

You can sort columns now with drag and drop

image

View the grid in full screen mode
image

Enjoy a much better way to change sorting

image

Expand documents in a flyout

image

You can resize one or multiple column, this is persisted in the saved search saved object and when you embed a saved search in a dashboard the width

Resolves #737
Resolves #38982
Resolves #8706
Resolved #4436

Known bugs and issues

  • EuiDataGrid - Wrong columns assigned to leadColumns (PR), so cell actions are displayed e.g. in the expand section of grid
  • EuiDataGrid Display sort arrow next in header column [EuiDataGrid] Add sorting icon if the column is sorted eui#4343 (comment)
  • EuiDataGrid - Cell actions spacing the Safari (issue)
  • EuiDataGrid - Resize handler overlapping header menu button at the rightmost column (issue)
  • EuiDataGrid - Cell actions need 2 clicks in Firefox and Safari on MacOS (issue)
  • EuiDataGrid - Cellpopover stays open even underlying data changes (issue)

Follow ups

Checklist

Michail Yasonik and others added 14 commits February 24, 2020 21:30
Merge remote-tracking branch 'upstream/master' into discover-data-grid
9eb57e6
@mbondyra
perf improvements
156a558
@snide
Merge branch 'discover-data-grid' of github.com:myasonik/kibana into …
8761909 
…discover-data-grid
@snide
basic layout and scrolling now works
f6b2cee
attempting to get tests to pass again
78a7405
fixed infinite scroll of legacy table
2bc70e9
fixing sorting and schema detection and other bugs
1702235
Merge remote-tracking branch 'upstream/master' into discover-data-grid
b3b873d
updating to use new state stuff
c372a0d
@snide
Merge remote-tracking branch 'upstream/master' into discover-data-grid
464eb22
@kertal
Merge master /fix conflicts
6443dea
@kertal
Merge commit 'dcfa4850098972b6b00fe869048e61602aea4683' into kertal-p…
6809cfc 
…r-2020-04-25-discover-data-grid
@kertal
Merge master /fix conflicts
5c97e03
@kertal
Activate DataGrid in embeddable
0819ae5
@kertal kertal self-assigned this May 25, 2020
@kertal kertal added the Feature:Discover Discover Application label May 25, 2020
@rayafratkina rayafratkina mentioned this pull request May 27, 2020
Closed
5 tasks
@kertal
Copy link
Member Author

kertal commented May 27, 2020

FYI: @myasonik & @snide

@andreadelrio andreadelrio mentioned this pull request Jun 4, 2020
Closed
@kertal
Copy link
Member Author

kertal commented Dec 18, 2020

Some notes.

  • We need to fix the spacing in the blob view. Right now there are no margins to the left of the blocks.
  • Selected state of rows needs emphasis

you took care of that, right @cchaos

  • We should add a toast or something to signify the actions in the flyouts happened. Since they are hidden behind.

Or we make them function like the legacy one, close the flyout, would also prevent a problem with toggling of columns

  • Products field is rendering empty in the grid, but shows in the flyout.

Muchas gracias, fixed that

  • In general, I think we need a schema to recognize JSON blobs, and apply that schema from EUI (for syntax highlighting in the popout)
  • We can set the height for the chart header so that there's not a jump as you hide the chart. (this is because the select form has a larger height)

Dankeschön for taking care of it

  • I would increase the default pagination to more than 25 results. It should be as high as we feel comfortable.

Will be done when virtualization, I'm comfortable with 5000000 then

about the geo field schema, so it should be handled like a JSON or differently? thx

@snide
Copy link
Contributor

snide commented Dec 18, 2020

@kertal did some more styling. We have some Amsterdam specific changes that we'll need to make (that shouldn't hold up this PR) after the break.

I'd still like to highlight the row selected if possible. Think you can pass me a className on the cells for the selected row? That way I can give it some emphasis once it's been expanded.

Other small thing I noticed in here (and can wait) is that we really need to replace the doc viewer table that shows in the expanded flyout to use an EuiTable. Right now it's using Bootstrap, and we're looking to remove that next minor.

@kertal kertal mentioned this pull request Dec 22, 2020
Closed
@kertal kertal mentioned this pull request Dec 22, 2020
Closed
@kertal kertal mentioned this pull request Dec 22, 2020
Closed
snide
snide approved these changes Dec 22, 2020
View reviewed changes
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. There are some small leftover issues from design, but they can all be handled by later PRs. All the big stuff is covered. When we get closer to a release, we should make a more prominent switch (maybe a one time toast?) to let people toggle the new view, that way they don't need to dig into adv. settings.

There's likely nearly ten people on this PR on the EUI or Kibana side. Really nice job by everyone. Good to merge!

@kertal kertal merged commit a417690 into elastic:master Dec 23, 2020
@majagrubic majagrubic mentioned this pull request Dec 23, 2020
Merged
majagrubic pushed a commit that referenced this pull request Dec 23, 2020
@kertal @mbondyra
@snide @andreadelrio
[Discover] Integration of EuiDataGrid (#67259) (#86871)
755b042 
Co-authored-by: Michail Yasonik <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
Co-authored-by: Dave Snider <[email protected]>
Co-authored-by: Andrea Del Rio <[email protected]>
Co-authored-by: cchaos <[email protected]>

Co-authored-by: Matthias Wilhelm <[email protected]>
Co-authored-by: Michail Yasonik <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
Co-authored-by: Dave Snider <[email protected]>
Co-authored-by: Andrea Del Rio <[email protected]>
Co-authored-by: cchaos <[email protected]>
@kibanamachine
Copy link
Contributor

kibanamachine commented Apr 20, 2021 •
edited
Loading

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [a98550b]

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kertal kertal mentioned this pull request May 26, 2021
Closed
@kertal kertal mentioned this pull request Oct 18, 2021
Closed
@timroes timroes mentioned this pull request Oct 20, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snide snide snide approved these changes

@stratoula stratoula stratoula approved these changes

@andreadelrio andreadelrio Awaiting requested review from andreadelrio

@sulemanof sulemanof Awaiting requested review from sulemanof

@cchaos cchaos Awaiting requested review from cchaos

Assignees

@kertal kertal

Labels
Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone